-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fabric8 deletion behaviour fix #2241
Conversation
@strimzi-ci run tests |
topic-operator/src/test/java/io/strimzi/operator/topic/TopicOperatorBaseIT.java
Outdated
Show resolved
Hide resolved
❗ Build Failed ❗ |
@strimzi-ci run tests |
❗ Build Failed ❗ |
@strimzi-ci run tests |
❗ Build Failed ❗ |
@strimzi-ci run tests |
✔️ Test Summary ✔️TEST_PROFILE: acceptance |
After a few tests runs it seems there is still an issue with deleting resources, so I reverted my reversion and added |
0e9a0bf
to
73ba533
Compare
Signed-off-by: Stanislav Knot <sknot@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. I guess all deleting operations were fixed (I didn't search for all of them tbh). :-)
@@ -256,7 +256,7 @@ public void podNameScopedCreateListGetDelete(Class<RT> cls, | |||
gotResource = mixedOp.apply(client).withName(pod.getMetadata().getName()).get(); | |||
assertThat(gotResource, is(nullValue())); | |||
|
|||
assertThat(mixedOp.apply(client).withName(pod.getMetadata().getName()).delete(), is(false)); | |||
assertThat(mixedOp.apply(client).withName(pod.getMetadata().getName()).cascading(true).delete(), is(false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the MockKube properly supports cascading deletion (i.e. I suspect it behaves the same for a cascading as a non-cascading delete), but if the tests pass I guess it doesn't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the test pass.
TBH, I'm a bit unsure abotu this.
The STs seem to have passed, so I'm fine with this as it does not seem to do any damage. But I'm not sure I understand this. |
I remember this "not so clear" modification was used in some topic ST. I tried to remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Jakub that (at least some of) these changes sholdn't really be necessary, but neither should they be harmful. And it's at least plausible that the cascading delete (even in the absence of child resources) make deletion synchronous, and thus a test more reliable.
Signed-off-by: Stanislav Knot sknot@redhat.com
Type of change
Description
Fixes #2223
After fabric8io/kubernetes-client#1807 was merged in fabric8 I think this bug should be fixed.
//edit: using 4.6.4 did not seem to help, using suggested
cascading(true)
where possible.Checklist
./design